Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feat] implented confirm password #31

Merged
merged 12 commits into from
Nov 20, 2024

Conversation

rachaelch3n
Copy link
Contributor

What's new in this PR 🧑‍🌾

Description

  • Separated the auth flow into two pages: /login and /signup.
  • Added a "confirm password" input on the signup page that displays an error if the password and confirm password inputs do not match.
  • Implemented the ability to show or hide password inputs for both "password" and "confirm password" fields.
  • Incorporated checks for password complexity (at least 8 characters, at least one number, and at least one lowercase character).
  • Disabled the signup button until there are no errors (password and confirm password match, and complexity checks are met).

Screenshots

No checks passed
Screen Shot 2024-11-04 at 2 26 22 AM
Some checks passed
Screen Shot 2024-11-04 at 2 27 26 AM
Complexity checks passed but passwords don't match
Screen Shot 2024-11-04 at 2 28 10 AM
All checks passed and signup button is enabled
Screen Shot 2024-11-04 at 2 28 34 AM
Toggle visibility
Screen Shot 2024-11-04 at 2 29 13 AM

How to review

  • Review the PasswordComplexity and PasswordInput implementation in utils
  • Test the login flow in login page.tsx and ensure signIn work as expected
  • Test the login flow in signup page.tsx and ensure signUp work as expected

Next steps

  • styling

Relevant links

Online sources

Related PRs

CC: @ccatherinetan

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this should be removed from the PR since we're only using pnpm? you could try running
git checkout main -- [path-to-package-lock.json]
then commit and push

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes! or u can directly just delete the entire file

password={password || ''} // Set default value if password is null
/>
{/* Password complexity requirements */}
{password && passwordComplexity === false && passwordComplexityError && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could simplify this to {password && !passwordComplexity && passwordComplexityError ...

const [passwordComplexityError, setPasswordComplexityError] = useState<
string | null
>(null);
const [passwordComplexity, setPasswordComplexity] = useState<boolean>(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be easier to change these variables to more descriptive names? lowkey got a bit confused haha, maybe isPasswordComplexityMet or something similar would help to differentiate it from passwordComplexityError

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point! agreed about renaming

@kylezryr
Copy link
Collaborator

looks great so far! waiting on comments from Catherine...

@ccatherinetan ccatherinetan linked an issue Nov 13, 2024 that may be closed by this pull request
Copy link
Collaborator

@ccatherinetan ccatherinetan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's looking so great rachael 🤩 ! added some more small nits, but mostly ready to go!

  1. add icons from figma
  2. correct placement of PasswordComplexity component
  3. remove null as a possible value for the password and confirmPassword states
  4. both PasswordInput.tsx and PasswordComplexity.tsx should be in /components rather than in /app/utils

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment on the AuthContext PR - let's comment this out and remove this file since we don't want this route to be active on the app rn.

app/utils.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's delete this file if it's empty!

function Requirement({ met, text }: { met: boolean; text: string }) {
return (
<p style={{ color: met ? 'green' : 'red' }}>
{met ? '✓' : '✗'} {text}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! this is all right for now, but we eventually want to use the icons from figma.
i'd recommend exporting greenCheck and redCross icons as svg's from figma and adding them to the project. If you rebase from main, you'll be able to use the icon component i added!

package.json Outdated
@@ -14,9 +14,10 @@
"pre-commit": "(pnpm run tsc || true) && (pnpm run lint || true) && pnpm run prettier"
},
"dependencies": {
"next": "^14.2.10",
"@next/swc-darwin-arm64": "^15.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line should be deleted!

name,
}) => {
return (
<div style={{ position: 'relative' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventually we want to replace the inline styling with Styled Components

)}
{/* Conditional password validation error message */}
<PasswordComplexity
password={password || ''} // Set default value if password is null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if password is not able to be of type null, we don't need this additional password || '' and we can simplify to password={password}

import React from 'react';

interface PasswordInputProps {
value: string | null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove the null typing!


interface PasswordInputProps {
value: string | null;
onChange: (e: React.ChangeEvent<HTMLInputElement>) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be simplified to
onChange: (password: string) => void
if that helps (as long as u change line 25 accordingly to
onChange={(e)=>onChange(e.target.value)}

<p style={{ color: 'red' }}>{passwordError}</p>
)}
{/* Conditional password validation error message */}
<PasswordComplexity
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while the "Passwords Do Not Match" error is in the correct place rn, to match designs, PasswordComplexity should be directly under the Password input (i.e. above Confirm Password)

name: string;
}

const PasswordInput: React.FC<PasswordInputProps> = ({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's change this to the standard format of component declaration:

export default function PassowordInput(
   {value, 
   onChange, 
   placeholder, 
   isVisible, 
   toggleVisibility, 
   name}:
   PasswordInputProps) {
...
}

setPasswordComplexityError(null); // Clear error if all conditions are met
} else if (password) {
setPasswordComplexity(false);
setPasswordComplexityError('Password must meet complexity requirements');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just flagging that it makes more sense for this error to only show up if user tries to submit without meeting requirements. it's fine for now but we may want to adjust the logic in the future


push('/');

return data;
};

return (
Copy link
Collaborator

@ccatherinetan ccatherinetan Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to make the outermost container a form to improve the accessibility of this page.

here's some skeleton code from chatGPT

import React, { useState } from "react";

export default function Signup() {
  const [email, setEmail] = useState("");
  const [password, setPassword] = useState("");
  const [confirmPassword, setConfirmPassword] = useState("");
  const [errorMessage, setErrorMessage] = useState("");

  const isFormValid = email && password && confirmPassword && password === confirmPassword;

  const handleSignUp = (e) => {
    e.preventDefault(); // Prevent default form submission behavior

    // Clear error message and proceed with sign-up logic
    setErrorMessage("");
    // Add your sign-up logic here
  };

  return (
    <form onSubmit={handleSignUp}>
      <input
        name="email"
        onChange={(e) => setEmail(e.target.value)}
        value={email}
        placeholder="Email"
        required
      />
      <input
        type="password"
        name="password"
        onChange={(e) => setPassword(e.target.value)}
        value={password}
        placeholder="Password"
        required
      />
      <input
        type="password"
        name="confirmPassword"
        onChange={(e) => setConfirmPassword(e.target.value)}
        value={confirmPassword}
        placeholder="Confirm Password"
        required
      />
      {errorMessage && <p style={{ color: "red" }}>{errorMessage}</p>}
      <button type="submit" disabled={!isFormValid}>
        Sign up
      </button>
    </form>
  );
}

@rachaelch3n rachaelch3n force-pushed the rachaelchen/21-implement-confirm-password branch from a542f70 to c2f53ae Compare November 20, 2024 05:28
@ccatherinetan ccatherinetan merged commit 96104b8 into main Nov 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Confirm Password
3 participants